react nodes #118
react nodes #118
Conversation
react/dependency installer/e4e1b2e6-26a1-4b3b-b472-4464814fa69a.json
Outdated
Show resolved
Hide resolved
react/dependency installer/e4e1b2e6-26a1-4b3b-b472-4464814fa69a.json
Outdated
Show resolved
Hide resolved
react/dependency installer/e4e1b2e6-26a1-4b3b-b472-4464814fa69a.json
Outdated
Show resolved
Hide resolved
react/dependency installer/e4e1b2e6-26a1-4b3b-b472-4464814fa69a.json
Outdated
Show resolved
Hide resolved
| "name": "error" | ||
| }, | ||
| "4c3736af-6977-4632-b048-5446eaa87edd": { | ||
| "type": "any", |
There was a problem hiding this comment.
Type of bounced should be the same as the type of sync.
You can formulate it like this:
typeof `sync`
There was a problem hiding this comment.
not sure need more details on what is meant?
There was a problem hiding this comment.
reviewing the command runner node. It is using the same types as have used although the error output type is now set to {"error": string} as suggested.
react/dependency installer/e4e1b2e6-26a1-4b3b-b472-4464814fa69a.json
Outdated
Show resolved
Hide resolved
react/react installer/e257d740-d47d-4794-82f1-3704850bdba9.json
Outdated
Show resolved
Hide resolved
react/react installer/e257d740-d47d-4794-82f1-3704850bdba9.json
Outdated
Show resolved
Hide resolved
react/react installer/e257d740-d47d-4794-82f1-3704850bdba9.json
Outdated
Show resolved
Hide resolved
react/react installer/e257d740-d47d-4794-82f1-3704850bdba9.json
Outdated
Show resolved
Hide resolved
| "6668cb7f-874b-4c24-ae30-89fe2e24bad1": { | ||
| "type": "any", | ||
| "name": "run", | ||
| "description": "Run npm installer if boolean value received is true.\n" |
There was a problem hiding this comment.
Now that the type of run is any, it runs on any received value, not just true, and the description should reflect that.
| "description": "Array of npm dependencies to install.\n\neg.\n\n[\"a\",\"b\"]" | ||
| }, | ||
| "045a62ad-7340-479a-b544-f10d56f16597": { | ||
| "type": "{\n\"projectPath\": string, \n\"start\": boolean\n}", |
There was a problem hiding this comment.
The start property in this input seems irrelevant as it's only purpose is to convert the projectPath param into a signal.
| "implementations": { | ||
| "es6-node": { | ||
| "inputFunctions": { | ||
| "045a62ad-7340-479a-b544-f10d56f16597": "const { exec } = require( \"child_process\" );\nconst { dependencies } = params;\nif (data.start !== false ){\n const command = \"cd \" + data.projectPath + \" && \" + \"npm install \" + params.dependencies.join(' ');\n outputs.stdout( command, tag );\n if (state.running){\n outputs.bounced(data, tag);\n } else {\n state.running = true;\n exec(command, (error, stdout, stderr) => {\n if (error) {\n outputs.error(`${error}`, tag);\n } else {\n outputs.stdout(stdout || \"\", tag);\n outputs.stderr(stderr || \"\", tag);\n outputs.error(null, tag);\n }\n state.running = false;\n });\n }\n}" |
There was a problem hiding this comment.
Here data.start !== false will always be satisfied, (when used from the main dependency installer node) as the parent node's syncer adds a run property, not start. I think this condition is unnecessary anyway, as the value carried by start/run is any.
| "implementations": { | ||
| "es6-node": { | ||
| "inputFunctions": { | ||
| "045a62ad-7340-479a-b544-f10d56f16597": "const { exec } = require( \"child_process\" );\nconst { dependencies } = params;\nif (data.start !== false ){\n const command = \"cd \" + data.projectPath + \" && \" + \"npm install \" + params.dependencies.join(' ');\n outputs.stdout( command, tag );\n if (state.running){\n outputs.bounced(data, tag);\n } else {\n state.running = true;\n exec(command, (error, stdout, stderr) => {\n if (error) {\n outputs.error(`${error}`, tag);\n } else {\n outputs.stdout(stdout || \"\", tag);\n outputs.stderr(stderr || \"\", tag);\n outputs.error(null, tag);\n }\n state.running = false;\n });\n }\n}" |
There was a problem hiding this comment.
Parameters stdout and stderr in the callback of exec can be Buffers, so I'd serialize them the same way as error is serialized.
Also I would not send null on error when there was no error.
| "name": "error" | ||
| }, | ||
| "4c3736af-6977-4632-b048-5446eaa87edd": { | ||
| "type": "any", |
There was a problem hiding this comment.
Should be "typeof `projectPath & start`" as it simply forwards that input.
| }, | ||
| "outputs": { | ||
| "86e1623a-d67c-4e74-98bb-449e29005d56": { | ||
| "type": "any", |
There was a problem hiding this comment.
Should be string as we know that's what we send from the code.
| "name": "stdout" | ||
| }, | ||
| "735430f5-61ba-48c6-a8a6-e952edcb259e": { | ||
| "type": "any", |
There was a problem hiding this comment.
Should be string as we know that's what we send from the code.
| "description": "Run npm installer if boolean value received is true.\n" | ||
| } | ||
| }, | ||
| "outputs": { |
There was a problem hiding this comment.
Need to makes sure types reflect those of the internal code node. (see comments below)
| "name": "react/React Dependency Installer", | ||
| "description": "Installs dependencies defined by the user for the react installer node. Node uses npm to install the dependencies on the same application path as the react application.", | ||
| "interface": { | ||
| "inputs": { |
There was a problem hiding this comment.
We should point out in the descriptions that dependencies and projectPath are not protected from injection attack. (Or validate in code that they are actually path and package names)
| @@ -0,0 +1,56 @@ | |||
| { | |||
There was a problem hiding this comment.
Most of the issues in the dependency installer repeat here, re. port types, serialization of stdout/stderr, etc.
Beyond that I feel the success output is redundant, as we either send something on stdout or error. If I need to turn that into a boolean, I can use a boolean/Reverse condition node.
One other thing I'd like to ask is to wrap this node into a structure node with an identical interface. As a general rule, we give preference to implementing nodes using existing nodes rather than JS, so this would keep the functionality of this node future-proof re. refactoring.
Hi,
I hope these are a welcomed addition to the repo!
react installer : Install a react application at a specified file path
react dependency installer : Install dependencies for a react application using npm
dependency installer : internal node for react dependency installer.
any changes or additions please let me know